Devin Fledermaus #44

DevinFledermaus · 2022-04-12T12:30:26Z

Onboarding Javascript

geraldtivatyi · 2022-04-28T07:12:34Z

app.ts

+const prevButton: any = document.querySelector("#prev");
+const clear = "";
+let paramOne: any = "0"
+let paramTwo: any = "9"


I think rather calculate this value based on the size of the window than have a fixed value. Screen sizes change and you can potentially fit more records on the screen. Try to fit as much readable records on the screen as possible.

anzelIMQS

Before I actually review the code, please add semicolons at the end of each line. You have a lot of them missing.

anzelIMQS

Function restructuring to remove the global parameters. These values should be passed around. Functions should not have to know about stuff happening outside of their scope.

anzelIMQS · 2022-05-03T07:45:01Z

app.ts

+        let currentStats = "Showing results from " + paramOne + " to " + paramTwo + " out of " + count + " results.";
+        pageStats.innerHTML = currentStats;
+      });
+  } catch (error) {}


Same as the above comment on the catch

anzelIMQS · 2022-05-03T07:45:40Z

app.ts

+  } catch (error) {
+    console.log(error);
+  }


I meant, a catch on the then of the Promise.
e.g. then( () => {...}).catch( (error) => {})

Not a seperate try and catch block.

anzelIMQS · 2022-05-03T07:46:58Z

app.ts

+  } catch (error) {
+    console.log(error);
+  }


Same as the above comment on the catch

anzelIMQS · 2022-05-03T07:48:08Z

app.ts

+      .then((data) => {
+        data = JSON.parse(data);
+        let count = data;
+        let currentStats = "Showing results from " + paramOne + " to " + paramTwo + " out of " + count + " results.";


Rather use Template Literals. ${}

anzelIMQS · 2022-05-03T07:49:39Z

app.ts

+        data = JSON.parse(data);
+        let count = data;


Probably not necessary to do this over 2 lines.

let count = JSON.parse(data);

anzelIMQS · 2022-05-03T08:08:41Z

app.ts

+  }
+};
+
+prev = prevDebounce(prev, 500);


Same vibe as the above next

anzelIMQS · 2022-05-03T08:09:32Z

app.ts

+  getTable();
+};
+
+idJump = debounce(idJump, 500);


Same vibe as the above next.

anzelIMQS · 2022-05-03T08:10:42Z

app.ts

+  };
+};
+
+let next = () => {


The paramOne and paramTwo should be accepted as parameters to this function.

anzelIMQS · 2022-05-03T08:11:04Z

app.ts

+  };
+};
+
+let prev = () => {


Same as mentioned in the next.

anzelIMQS · 2022-05-03T08:11:39Z

app.ts

+    paramTwo = 999999;
+    paramOne = paramTwo - getNoOfRows();
+  } else {
+    paramOne;


Remove redundancy.

geraldtivatyi · 2022-05-03T14:34:57Z

app.ts

+  let toParameter: number;
+  const height = window.innerHeight;
+
+  let noOfRows = Math.floor(height / 40);


What happens if you rather call the method getNoOfRows which you created, instead of repeating the calculation here. This will also mean you won't need the variable height.

anzelIMQS

A few more changes regarding functions.

anzelIMQS · 2022-05-05T07:41:24Z

app.ts

+    } else {
+      //pass
+    }


I mean. You can completely remove this else since is does nothing.

anzelIMQS · 2022-05-05T07:52:47Z

app.ts

+
+// Displays The Current Results Being Shown
+
+const stats = () => {


What I mean by recordCount is constant is that you don't have to make an API call every time you call stats since the value is going to be the same for each call.

In this case, I recommend you create a global const variable that you assign the recordCount API response once on the initial load of your program. At the moment it is a hardcoded value. Let the backend determine it.

anzelIMQS · 2022-05-05T08:03:26Z

app.ts

+      getParameters((fromParameter = 0));
+    })
+    .catch((error) => {
+      console.log("Call Hector", error);


Who's Hector?

anzelIMQS · 2022-05-05T08:05:33Z

app.ts

+  clearTable();
+  getTable();
+  stats();


I can definitely see these 3 functions actually being in one as it always goes together logically. They are always repeated in that order and are not too complex to be split up. It can be all together in getTable()?

anzelIMQS · 2022-05-05T08:08:24Z

app.ts

+//// Debounce
+
+const debounce = (fn: any, delay: number) => {
+  let timer: number;


You might need to add timer as a global variable. It's something I noticed with Ashton as well and he got it to work. At this stage, timer is only in the scope of this function.

anzelIMQS

More cleaning up, please.

anzelIMQS · 2022-05-05T11:51:54Z

app.ts

+  let end = fromParameter + getNoOfRows();
+  let toParameter: number;
+
+  if (end > 999999) {


Don't use 999999. You have recordCount for the max..

anzelIMQS · 2022-05-05T11:52:17Z

app.ts

+// getRecordCount();
+// getHeadings();
+// getTable();


Remove clutter.

anzelIMQS · 2022-05-05T11:53:21Z

app.ts

+  let next = () => {
+    let toParameter = getParameters(fromParameter);
+
+    if (toParameter === 999999) {


Same vibe for recordCount

anzelIMQS · 2022-05-05T11:53:28Z

app.ts

+    if (end > 999999) {
+      toParameter = 999999;


Same vibe for recordCount

anzelIMQS · 2022-05-05T11:55:13Z

app.ts

+  console.log("CALL HECTOR!!!");
+};
+
+const getParameters = (fromParameter: number) => {


Add return type

anzelIMQS · 2022-05-05T11:55:29Z

app.ts

+  return toParameter;
+};
+
+const getNoOfRows = () => {


Add return type

anzelIMQS · 2022-05-05T11:59:56Z

app.ts

+        fromParameter = intOne;
+      }
+
+      toParameter = fromParameter + getNoOfRows();


Is this still necessary? toParameter isn't used after this?

anzelIMQS · 2022-05-05T12:00:27Z

app.ts

+    if (search !== NaN && search !== "" && search < 1000000 && search >= 0) {
+      if (end > 999999) {
+        toParameter = 999999;


Same vibe for recordCount.

anzelIMQS · 2022-05-05T12:02:22Z

app.ts

+      for (let i = 0; i < headingData.length; i++) {
+        createHeadingRow(headingData[i]);
+      }
+      getParameters((fromParameter = 0));


Was this necessary? getParameters returns your toParameter but you do nothing with the return result since you don't need it?

FritzOnFire

Please convert all spaces to tabs. Please change the functions that you defined as variables to be defined as normal functions, you can leave the ones that you actually override as they were.

I made a comment about removing the empty line between the comment and the variable, there are a few places that this should happen.

Its a bit hard to review as it is, please make the changes where ever you think they are relevant and then I will review again.

FritzOnFire · 2022-05-06T06:01:00Z

app.ts

+// Heading Row
+
+const createHeadingRow = (headingData: string) => {


Rather remove the empty line between the variable and the comment.

FritzOnFire · 2022-05-06T06:02:25Z

app.ts

+// Heading Row
+
+const createHeadingRow = (headingData: string) => {
+  const heading: any = document.querySelector("#heading");


The only place you can have an any in the project is when handling data that comes from the back-end. And even then you can actually use the real type instead of any.

FritzOnFire · 2022-05-06T06:05:04Z

app.ts

+// Heading Row (Getting the columns data)
+
+const getHeadings = () => {
+  fetch("http://localhost:2050/columns", {


You should put a return in front of the fetch, that would allow other functions to wait on this function.

FritzOnFire · 2022-05-06T06:06:03Z

app.ts

+      for (let i = 0; i < headingData.length; i++) {
+        createHeadingRow(headingData[i]);
+      }
+      getParameters((fromParameter = 0));


To many braces and I would rather have this as two lines, where you explicitly call getParamter with a hard coded 0

FritzOnFire · 2022-05-06T06:07:31Z

app.ts

+    method: "GET",
+    headers: { "Content-Type": "application/json" },
+  })
+    .then((res) => res.text())


You should check for an error in res here. And you either want to remove the () around the res or specify the type, because currently the compiler thinks the type is any

FritzOnFire · 2022-05-06T06:08:28Z

app.ts

+      }
+      getParameters((fromParameter = 0));
+    })
+    .catch((error) => {


You want to remove the () around the error or specify the type, as currently the compiler thinks the type is any

FritzOnFire

Please change your spaces to tabs

FritzOnFire · 2022-05-09T08:37:56Z

app.ts

+
+// Loads all intial fields/data
+
+window.onload = function () {


FritzOnFire · 2022-05-09T08:38:26Z

app.ts

+        .then(() => {
+            return getTable();
+        });
+    console.log("CALL HECTOR!!!");


Stray console log.

FritzOnFire · 2022-05-09T08:39:07Z

app.ts

+    console.log("CALL HECTOR!!!");
+};
+
+function getParameters(fromParameter: number) {


Please add a return type

FritzOnFire · 2022-05-09T08:40:58Z

app.ts

+}
+
+// Table Content
+function createTableContent(contentData: string) {


They way you are using contentData gives me the idea that it is not simply a string -_-

FritzOnFire · 2022-05-09T08:42:29Z

app.ts

+    const content: HTMLElement | null = document.getElementById("content");
+
+    let table = `<div id="row-${contentData[0]}" class="rows"></div>`;
+    if (content !== null) {


You probably want to stop the function at this point, and tell the user something is wrong, as the rest of this function is dependent on this having a value.

FritzOnFire · 2022-05-09T08:51:47Z

app.ts

+        toParameter = maxRecordsID;
+        fromParameter = toParameter - getNoOfRows();
+    } else {
+        toParameter = fromParameter + getNoOfRows();


What is the purpose of this line?

FritzOnFire · 2022-05-09T08:52:17Z

app.ts

+    getTable();
+}
+
+window.addEventListener("resize", debounce(resizing, 500));


This line should not be in global space.

FritzOnFire · 2022-05-09T08:53:00Z

app.ts

+{
+    let nextCount = 0;
+    const nextButton: HTMLElement | null = document.getElementById("next");
+
+    const nextDebounce = (fn: any, delay: number) => {
+        return function () {
+            nextCount++;
+            clearTimeout(timer);
+            timer = setTimeout(() => {
+                fn();
+            }, delay);
+        };
+    };
+
+    let next = () => {
+        let toParameter = getParameters(fromParameter);
+        let maxRecordsID = recordCount - 1;
+
+        if (toParameter === maxRecordsID) {
+            alert("You have reached the final page");
+        }
+
+        let nextAmount = toParameter - fromParameter + 1;
+        let nextCountAmount = nextAmount * nextCount;
+        fromParameter = fromParameter + nextCountAmount;
+        toParameter = fromParameter + getNoOfRows();
+
+        let end = fromParameter + getNoOfRows();
+
+        if (end > maxRecordsID) {
+            toParameter = maxRecordsID;
+            fromParameter = toParameter - getNoOfRows();
+        }
+
+        nextCount = 0;
+
+        getTable();
+    };
+
+    if (nextButton !== null) {
+        nextButton.addEventListener("click", nextDebounce(next, 500));
+    }
+}
+
+// Previous
+{
+    let prevCount = 0;
+    const prevButton: HTMLElement | null = document.getElementById("prev");
+
+    const prevDebounce = (fn: any, delay: number) => {
+        return function () {
+            prevCount++;
+            clearTimeout(timer);
+            timer = setTimeout(() => {
+                fn();
+            }, delay);
+        };
+    };
+
+    let prev = () => {
+        let toParameter = getParameters(fromParameter);
+
+        if (fromParameter === 0) {
+            alert("You Are On The First Page");
+        } else {
+            let prevAmount = toParameter - fromParameter + 1;
+            let prevCountAmount = prevAmount * prevCount;
+
+            let intOne = fromParameter - prevCountAmount;
+
+            if (intOne < 0) {
+                fromParameter = 0;
+            } else {
+                fromParameter = intOne;
+            }
+
+            prevCount = 0;
+
+            getTable();
+        }
+    };
+
+    if (prevButton !== null) {
+        prevButton.addEventListener("click", prevDebounce(prev, 500));
+    }
+}
+
+// ID Jump
+{
+    const input: any = document.querySelector("input");
+
+    let idJump = () => {
+        let toParameter = getParameters(fromParameter);
+        let currentID = fromParameter;
+        let search = input.value;
+        let end = parseInt(search) + getNoOfRows();
+        let maxRecordsID = recordCount - 1;
+
+        if (search !== NaN && search !== "" && search <= maxRecordsID && search >= 0) {
+            if (end > maxRecordsID) {
+                toParameter = maxRecordsID;
+                fromParameter = toParameter - getNoOfRows();
+            } else {
+                fromParameter = parseInt(search);
+                toParameter = fromParameter + getNoOfRows();
+            }
+        } else if (search !== "") {
+            alert("Make Sure Your Desired ID Is Not A Negative Number Or Doesn't Exceed 999999");
+            fromParameter = currentID;
+            toParameter = fromParameter + getNoOfRows();
+            input.value = "";
+        }
+
+        getTable();
+    };
+
+    window.addEventListener("input", debounce(idJump, 500));
+}


This is a lot of logic living in global space, either move it to a class, or execute it somewhere else.

FritzOnFire · 2022-05-09T08:53:50Z

index.html

+        <title>Onboarding JavaScript Task</title>
+        <script type="text/javascript" charset="utf-8" src="third_party/jquery-2.0.3.min.js"></script>
+        <link rel="stylesheet" href="style.css" />
+        <script src="app.js" defer></script>


You really should not be using defer in the project, but there are no rules against it, so I'm not going to ask you to remove it.

FritzOnFire · 2022-05-09T08:56:49Z

style.css

+button {
+  border: none;
+  padding: 5px 10px;
+  border-radius: 10px;
+  width: 100px;
+  color: #000;
+  background-color: #fff;
+  cursor: pointer;
+  transition: all 0.3s;
+  transition-timing-function: ease-in-out;
+}
+button:hover {
+  letter-spacing: 1px;
+  color: #fff;
+  background-color: #000;
+  border: 1px solid #fff;
+}


Nice touch :)

FritzOnFire · 2022-05-11T06:57:55Z

style.css

@@ -0,0 +1,115 @@
+* {
+  padding: 0;


Please convert the spaces to tabs on this file.

FritzOnFire · 2022-05-11T06:58:36Z

app.ts

+
+// Loads all intial fields/data
+
+window.onload = function () {


FritzOnFire · 2022-05-11T07:00:30Z

app.ts

+	const next = new Next(); // next class
+	const prev = new Prev(); // prev class
+	window.addEventListener("input", debounce(idJump, 500));
+	window.addEventListener("resize", debounce(resizing, 500));
+	getRecordCount()
+		.then(() => {
+			return getHeadings();
+		})
+		.then(() => {
+			return getTable();
+		});


The classes and functions that you are using here should be defined before you use them.

FritzOnFire · 2022-05-11T08:16:55Z

app.ts

+	} else {
+		alert("ERROR");
+	}


You probably also want to return in this else as the rest of your code doesn't really work if the content does not exist.

FritzOnFire · 2022-05-11T08:18:54Z

app.ts

+}
+
+// Table Content
+function createTableContent(contentData: any) {


Please specify the type of contentData. If you can use it int a for-loop then you at the very least know its an array.

FritzOnFire · 2022-05-11T08:45:39Z

app.ts

+				clearTimeout(timer);
+				timer = setTimeout(() => {


You should add this timer (with a different name) to the class, then you are less depended on the global variables.

FritzOnFire · 2022-05-11T08:50:13Z

app.ts

+		};
+
+		if (this.prevButton) {
+			this.prevButton.addEventListener("click", prevDebounce(prev, 500));


Same comment about the debounce that I made on the Next class, should help you get rid of the prevCount.

FritzOnFire · 2022-05-11T08:50:57Z

app.ts

+				clearTimeout(timer);
+				timer = setTimeout(() => {


Same comment about the timer I made in the Next class

FritzOnFire · 2022-05-11T08:51:59Z

app.ts

+
+// ID Jump
+function idJump() {
+	const input: any = document.querySelector("input");


Please don't use any.

FritzOnFire · 2022-05-11T08:53:17Z

style.css

+#content {
+  display: flex;
+  flex-direction: column;
+  height: calc(100vh - 15vh);


calc is bad, don't use calc

geraldtivatyi · 2022-05-11T09:58:33Z

app.ts

+
+// Table Content (Getting the table's data)
+function getTable(): Promise<void> {
+	const content: HTMLElement | null = document.getElementById("content");


I think there is a rule about how to use document.getElementById in the Javascript Style Guide.

Sorry man, I just saw that, I'm removing that section from the style guide :P Had a discussion with cobus a while back, and we preferred the more pure route.

No problem!!

FritzOnFire

Some small changes, but I think you are done.

FritzOnFire · 2022-05-12T06:31:35Z

app.ts

@@ -0,0 +1,286 @@
+let fromParameter = 0;
+let recordCount: number;
+const el = document.getElementById


Please remove this.

FritzOnFire · 2022-05-12T06:32:55Z

app.ts

+}
+
+// Table Content
+function appendTableContent(contentData: string | string[]) {


The way your code is written it should definitely only be string[]

FritzOnFire · 2022-05-12T06:46:17Z

app.ts

+	})
+		.then(handleErrors)
+		.then((response: Response) => response.json())
+		.then((headingData: string | string[]) => {


The way your code is written it should definitely only be string[]

FritzOnFire · 2022-05-12T06:47:42Z

app.ts

+	})
+		.then(handleErrors)
+		.then((res: Response) => res.json())
+		.then((contentData: string | string[]) => {


They way your code is written... neither of these are right. Based off what you get from the back end, should it not be a 2D array?

FritzOnFire · 2022-05-12T06:49:47Z

app.ts

+			return () => {
+				clearTimeout(this.nextTimer);
+				this.nextTimer = setTimeout(() => {
+					fn();
+				}, delay);
+			};


I think you should rather change this to not return a function, that way you don't have to add that weird () on line 189.

FritzOnFire · 2022-05-12T06:51:21Z

app.ts

+// Previous
+class Prev {
+	prevButton: HTMLElement | null;
+	prevTimer: number = 0


Missing ;

FritzOnFire · 2022-05-12T06:51:53Z

app.ts

+		const prevDebounce = (fn: () => void, delay: number) => {
+			return () => {
+				clearTimeout(this.prevTimer);
+				this.prevTimer = setTimeout(() => {
+					fn();
+				}, delay);
+			};
+		};


I think you should rather change this to not return a function, that way you don't have to have that weird () on line 231.

FritzOnFire · 2022-05-12T06:52:00Z

app.ts

+				fromParameter = toParameter - getNoOfRows();
+			}
+
+			nextDebounce(getTable, 500)()


missing ;

FritzOnFire · 2022-05-12T06:52:09Z

app.ts

+// Next
+class Next {
+	nextButton: HTMLElement | null;
+	nextTimer: number = 0


missing ;

FritzOnFire · 2022-05-12T06:52:24Z

app.ts

+					fromParameter = intOne;
+				}
+
+				prevDebounce(getTable, 500)()


Missing ;

FledermausDevin added 23 commits April 12, 2022 12:00

final push

88a0c43

final push

69a305f

final push

95cabc6

final push

5f81214

final push

75bff11

final push

3b59290

revised code attempt 1

6de1c55

revised code attempt 1

7bd7bc8

next and prev buttons fixed

7991e74

next and prev buttons fixed

c54940f

next and prev buttons fixed

3e5eae5

revised code

b028cc4

revised code

dfc1c75

window resizing

6c53b8d

window resizing

6a9233b

window resizing

708034d

resize

027f290

debounce implimented

c4039ef

debounce implimented

ecceaf0

debounce implimented

5ff3563

column aligning

405592d

column aligning

c67502c

final testing complete

7cf9e78

DevinFledermaus requested review from CelesteNaude, FritzOnFire, anzelIMQS and geraldtivatyi April 26, 2022 13:09

geraldtivatyi reviewed Apr 28, 2022

View reviewed changes

anzelIMQS requested changes Apr 28, 2022

View reviewed changes

final testing complete

708ef31

DevinFledermaus requested review from CelesteNaude and anzelIMQS April 29, 2022 09:48

revision after code review

93e2511

anzelIMQS requested changes May 3, 2022

View reviewed changes

FledermausDevin added 5 commits May 3, 2022 14:06

03 May code review revision

0b1c2e6

03 May code review revision

876307b

03 May code review revision

d2cf1df

03 May code review revision

e31c2d3

03 May code review revision

f5fdd2d

geraldtivatyi reviewed May 3, 2022

View reviewed changes

4 May Code Review Implimented

951158c

anzelIMQS requested changes May 5, 2022

View reviewed changes

05 May code revisied

46fc0eb

anzelIMQS requested changes May 5, 2022

View reviewed changes

FledermausDevin added 2 commits May 5, 2022 14:27

05 May code revisied

b581516

05 May code revisied

4d5c1a5

FritzOnFire requested changes May 6, 2022

View reviewed changes

FledermausDevin added 2 commits May 6, 2022 08:57

06 May code revisied

f2457da

06 May code revisied

ade9748

FritzOnFire requested changes May 9, 2022

View reviewed changes

FledermausDevin added 4 commits May 9, 2022 14:27

09 May code review

cdaf8aa

09 May code review

b72ecd9

09 May code review

7b4e7a6

11 May update

62417f3

FritzOnFire requested changes May 11, 2022

View reviewed changes

11 May update

d3ad477

geraldtivatyi reviewed May 11, 2022

View reviewed changes

11 May update

e82ac48

FritzOnFire requested changes May 12, 2022

View reviewed changes

12 May - Final Changes

4bdde48


		// Displays The Current Results Being Shown

		const stats = () => {

		// Heading Row

		const createHeadingRow = (headingData: string) => {


		// Loads all intial fields/data

		window.onload = function () {

Devin Fledermaus #44

Are you sure you want to change the base?

Devin Fledermaus #44

Uh oh!

Conversation

DevinFledermaus commented Apr 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anzelIMQS left a comment

Choose a reason for hiding this comment

Uh oh!

anzelIMQS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anzelIMQS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anzelIMQS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FritzOnFire left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

FritzOnFire left a comment •

edited

Loading